Closed Bug 1098074 Opened 7 years ago Closed 7 years ago

Remove the ReportPendingException in nsFrameMessageManager::ReceiveMessage

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(3 files)

And add some missing error handling.
Attachment #8521872 - Flags: review?(bobbyholley)
Attachment #8521870 - Flags: review?(bobbyholley) → review+
Comment on attachment 8521872 [details] [diff] [review]
nsframemessagemanager_2-v0.diff

Review of attachment 8521872 [details] [diff] [review]:
-----------------------------------------------------------------

My last review on this got lost :-(

Please switch this to:

bool ok = JS_Foo() && JS_Bar();
NS_ENSURE_TRUE(ok, NS_ERROR_FAILURE);

So that we're consistent in error handling style between XPCOM and SM calls.
Attachment #8521872 - Flags: review?(bobbyholley) → review+
And, finally, a test poking the new error code.
Attachment #8522527 - Flags: review?(bobbyholley)
Comment on attachment 8522527 [details] [diff] [review]
nsframemessagemanager_3-v0.diff

Review of attachment 8522527 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks a million for adding all these tests!

::: dom/base/test/chrome/test_bug1098074_throw_from_ReceiveMessage.xul
@@ +26,5 @@
> +  var globalMM = Cc["@mozilla.org/globalmessagemanager;1"]
> +                   .getService(Ci.nsIMessageListenerManager);
> +  globalMM.addMessageListener("flimfniffle", function onMessage(msg) {
> +    globalMM.removeMessageListener("flimfniffle", onMessage);
> +    is(msg.data, "toofeltor", "correct message");

In german I'd read this as a slight misspelling of "Devil Door" - intentional? ;-)

@@ +47,5 @@
> +  <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=1098074"
> +     target="_blank">Mozilla Bug 1098074</a>
> +  </body>
> +
> +  <browser messagemanagergroup="test1" type="content-targetable" src="about:mozilla" />

I'm not sure about this part, but it looks cribbed from other tests, so it's probably right?
Attachment #8522527 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (:bholley) from comment #4)
> ::: dom/base/test/chrome/test_bug1098074_throw_from_ReceiveMessage.xul
> @@ +26,5 @@
> > +  var globalMM = Cc["@mozilla.org/globalmessagemanager;1"]
> > +                   .getService(Ci.nsIMessageListenerManager);
> > +  globalMM.addMessageListener("flimfniffle", function onMessage(msg) {
> > +    globalMM.removeMessageListener("flimfniffle", onMessage);
> > +    is(msg.data, "toofeltor", "correct message");
> 
> In german I'd read this as a slight misspelling of "Devil Door" -
> intentional? ;-)

Yes, it is a town in a D&D campaign my friend is running. Initially I used foobar here, however, I noticed when cribbing from other tests that everyone else is too. Since we're explicitly testing the error paths, I didn't want to trip over any cases where someone used a placeholder string either in firefox, or maybe racing with another test.

> @@ +47,5 @@
> > +  <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=1098074"
> > +     target="_blank">Mozilla Bug 1098074</a>
> > +  </body>
> > +
> > +  <browser messagemanagergroup="test1" type="content-targetable" src="about:mozilla" />
> 
> I'm not sure about this part, but it looks cribbed from other tests, so it's
> probably right?

Yes it's cribbed; unfortunately, I was never able to get the environment correct enough for the messageManager property to show up on those browser windows though. Using the main globalMM hits the same code path, however, so I don't think it matters. I will remove the extra browser windows as it seems they are unnecessary.
You need to log in before you can comment on or make changes to this bug.