Fix missing webNavigation onCommitted event on iframes loading

RESOLVED FIXED in Firefox 48

Status

defect
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: rpl, Assigned: rpl)

Tracking

(Blocks 1 bug)

unspecified
mozilla48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [webNavigation])

Attachments

(1 attachment)

Assignee

Description

3 years ago
In the process of raising up the coverage of scenarios reproduced in webNavigation tests, the attached patch is going to introduce:

- expand the webnavigation test case to check for the exact expected sequence of events for both the top-level frame and sub-frames

- fix issues with the missing LocationChange webprogress events related to iframe loading (which turns into missing onCommitted webNavigation events for the iframes on first load)
Assignee

Updated

3 years ago
Whiteboard: [webNavigation]
Assignee

Updated

3 years ago
Attachment #8738982 - Flags: review?(gkrizsanits)
Attachment #8738982 - Flags: feedback?(aswan)
Assignee

Updated

3 years ago
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Comment on attachment 8738982 [details]
MozReview Request: Bug 1262794 - [webext] webNavigation refactoring and fix missing onCommitted event on iframes loading. r=krizsa

https://reviewboard.mozilla.org/r/45003/#review42271

::: toolkit/components/extensions/test/mochitest/test_ext_webnavigation.html:153
(Diff revision 1)
> +  // As required in the webNavigation API documentation:
> +  //   If a navigating frame contains subframes, its onCommitted is fired before any
> +  //   of its children's onBeforeNavigate; while onCompleted is fired after
> +  //   all of its children's onCompleted.

Is this commenting indentation intentional?

::: toolkit/components/extensions/test/mochitest/test_ext_webnavigation.html:160
(Diff revision 1)
> +  // As required in the webNAvigation API documentation, check the event sequence:
> +  //   onBeforeNavigate -> onCommitted -> onDOMContentLoaded -> onCompleted

Same here.

::: toolkit/modules/addons/WebNavigationContent.js:112
(Diff revision 1)
>        } else if (loadType & Ci.nsIDocShell.LOAD_CMD_HISTORY) {
>          isHistoryStateUpdated = true;
>        }
>      }
>  
> +    if (isHistoryStateUpdated || isReferenceFragmentUpdated) {

Could you explain this line? I don't understand what is the intention here. Why don't we want to propogate the location change if the document changes? And if the document is the same and both these flags are false... why did we get the location change notification?
Attachment #8738982 - Flags: review?(gkrizsanits)
https://reviewboard.mozilla.org/r/45003/#review42375

The original bug references "issues with the missing LocationChange events", but it isn't clear to me from the patch what those issues are, are there further tests that could be added for these issues?  (or are the issues the event ordering which you do address here?)

::: toolkit/modules/addons/WebNavigationContent.js:69
(Diff revision 1)
>      sendAsyncMessage("Extension:StateChange", data);
>  
> -    if (webProgress.DOMWindow.top != webProgress.DOMWindow) {
> +    if (stateFlags & Ci.nsIWebProgressListener.STATE_IS_DOCUMENT) {
> -      let webNav = webProgress.QueryInterface(Ci.nsIWebNavigation);
> -      if (!webNav.canGoBack) {
> -        // For some reason we don't fire onLocationChange for the
> +      // For some reason we don't fire onLocationChange for the

If the problem is that webprogress does not invoke onLocationChange, can this comment be updated to say that more clearly?  Also, if there is a bug for that problem, it would be good to include a reference to it here so that when somebody fixes that bug and these tests start failing, it is clear that the workaround can be removed.

::: toolkit/modules/addons/WebNavigationContent.js:124
(Diff revision 1)
>  
> -    sendAsyncMessage("Extension:LocationChange", data);
> +      sendAsyncMessage("Extension:LocationChange", data);
> +    }
> +  },
> +
> +  processStateDocument({webProgress, request, locationURI, flags}) {

If this is just used for the workaround for missing events on sub-frames, perhaps the method name could be updated to reflect that?  This name didn't make much sense to me.
Attachment #8738982 - Flags: feedback?(aswan)
Assignee

Comment 4

3 years ago
https://reviewboard.mozilla.org/r/45003/#review42271

> Could you explain this line? I don't understand what is the intention here. Why don't we want to propogate the location change if the document changes? And if the document is the same and both these flags are false... why did we get the location change notification?

By adding more scenarios into the existent test files of the webNavigation API,
I noticed that the previous workaround which should be able to catch the missing LocationChange from the sub-frames:

>  if (webProgress.DOMWindow.top != webProgress.DOMWindow) {
>    let webNav = webProgress.QueryInterface(Ci.nsIWebNavigation);
>    if (!webNav.canGoBack) {
>      // For some reason we don't fire onLocationChange for the
>      // initial navigation of a sub-frame. So we need to simulate
>      // it here.
>      this.onLocationChange(webProgress, request, request.QueryInterface(Ci.nsIChannel).URI, 0);
>    }

is not working correctly and we still have missing location changes events on sub-frames (which then turns into missing onCommitted webNavigation events), and by reading again the documentation it turned out the onCommitted message should be raise when frame is loading the document:

    [onCommitted](https://developer.chrome.com/extensions/webNavigation#event-onCommitted)
    Fired when a navigation is committed. The document (and the resources it refers to, 
    such as images and subframes) might still be downloading, but at least part of 
    the document has been received...
    
This has made me think that probably it would be better to catch the onCommitted phase of the webNavigation events sequence from the onStateChange handler for all the cases (instead of using onLocationChange only for the top level frames) to better uniforming how the onCommitted phase is handled.

Unfortunately when a frame navigation doesn't change the current loaded document (which can be due to history.pushState/replaceState or to a changed hash in the url) are reported only to the onLocationChange, for this reason we still need that method to handle that scenarios (which will always happen after the first load, and so it works correctly for both top-level frames and sub-frames).
Assignee

Comment 5

3 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #2)
> Comment on attachment 8738982 [details]
> MozReview Request: Bug 1262794 - [webext] fix missing webNavigation
> onCommitted event on iframes loading.
> 
> https://reviewboard.mozilla.org/r/45003/#review42271
> 
> :::
> toolkit/components/extensions/test/mochitest/test_ext_webnavigation.html:153
> (Diff revision 1)
> > +  // As required in the webNavigation API documentation:
> > +  //   If a navigating frame contains subframes, its onCommitted is fired before any
> > +  //   of its children's onBeforeNavigate; while onCompleted is fired after
> > +  //   all of its children's onCompleted.
> 
> Is this commenting indentation intentional?
> 
> :::
> toolkit/components/extensions/test/mochitest/test_ext_webnavigation.html:160
> (Diff revision 1)
> > +  // As required in the webNAvigation API documentation, check the event sequence:
> > +  //   onBeforeNavigate -> onCommitted -> onDOMContentLoaded -> onCompleted
> 
> Same here.

I indented the part of the two comments that are quotes from the API reference & docs, 
but I'm going to indent it as the rest of the comment:
if it is not self-explanatory then it means that indenting it that way
to highlight that is a quote don't work very well.
Assignee

Comment 6

3 years ago
Comment on attachment 8738982 [details]
MozReview Request: Bug 1262794 - [webext] webNavigation refactoring and fix missing onCommitted event on iframes loading. r=krizsa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45003/diff/1-2/
Attachment #8738982 - Attachment description: MozReview Request: Bug 1262794 - [webext] fix missing webNavigation onCommitted event on iframes loading. → MozReview Request: Bug 1262794 - [webext] webNavigation refactoring and fix missing onCommitted event on iframes loading.
Assignee

Comment 7

3 years ago
Comment on attachment 8738982 [details]
MozReview Request: Bug 1262794 - [webext] webNavigation refactoring and fix missing onCommitted event on iframes loading. r=krizsa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45003/diff/2-3/
Assignee

Comment 8

3 years ago
Comment on attachment 8738982 [details]
MozReview Request: Bug 1262794 - [webext] webNavigation refactoring and fix missing onCommitted event on iframes loading. r=krizsa

After a brief discussion on IRC with Andrew, we agreed that a more descriptive inline comment based on Comment 4 will make the change much more clear.

Besides that, we discussed about the MessageManager message names, which doesn't match anymore their usage inside the WebProgress listener.

I agree with Andrew that using better matching MessageManager message names can help to make their role much more clear,
and so I prepared a new version which use the following message names:

- "Extension:StateChange" (no changes from the current version)
- "Extension:DocumentChange" (generate the onCommitted event, receive updates when the state change verify the condition
  'stateFlags & Ci.nsIWebProgressListener.STATE_IS_DOCUMENT') 
- "Extension:HistoryChange" (generate the onHistoryStateUpdated and the onReferenceFragmentUpdated, receive updates when the onLocationChange listener verify a navigation that does not change the document)
Attachment #8738982 - Flags: review?(gkrizsanits)
Attachment #8738982 - Flags: feedback?(aswan)
https://reviewboard.mozilla.org/r/45003/#review42869

looks much clearer, i'll take a more detailed look in the morning.
Comment on attachment 8738982 [details]
MozReview Request: Bug 1262794 - [webext] webNavigation refactoring and fix missing onCommitted event on iframes loading. r=krizsa

https://reviewboard.mozilla.org/r/45003/#review42931

This looks great. I know it does not belong to this patch but was wondering whay onReferenceFragmentUpdated is actually doing? The docs say "Fired when the reference fragment of a frame was updated" but that's not clear to me at all... Anyway r+ and thanks for the comments!
Attachment #8738982 - Flags: review?(gkrizsanits) → review+
Assignee

Comment 11

3 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #10) 
> I know it does not belong to this patch but was wondering
> whay onReferenceFragmentUpdated is actually doing? The docs say "Fired when
> the reference fragment of a frame was updated" but that's not clear to me at
> all...

But it is a more than reasonable question (and the chrome docs doesn't say enough about it):
the onReferenceFragmentUpdated event is sent when, during a navigation, the only change is in the hash part of the current location url (when it has not been generated by an history.pushState/replaceState)
Assignee

Updated

3 years ago
Iteration: --- → 48.3 - Apr 25
Assignee

Comment 12

3 years ago
Comment on attachment 8738982 [details]
MozReview Request: Bug 1262794 - [webext] webNavigation refactoring and fix missing onCommitted event on iframes loading. r=krizsa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45003/diff/3-4/
Attachment #8738982 - Attachment description: MozReview Request: Bug 1262794 - [webext] webNavigation refactoring and fix missing onCommitted event on iframes loading. → MozReview Request: Bug 1262794 - [webext] webNavigation refactoring and fix missing onCommitted event on iframes loading. r=krizsa
Attachment #8738982 - Flags: feedback?(aswan)
Assignee

Comment 13

3 years ago
https://reviewboard.mozilla.org/r/45001/#review43027

During the rebase of the other patches that are going to be based on this change, I noticed that if we always notify the "onCommitted" event from the "onStateChange" method, we are not going to be able to detect the "server_redirect" transitionQualifier, because the onCommitted event will be sent before the redirect can be detected.

::: toolkit/modules/addons/WebNavigationContent.js:61
(Diff revisions 3 - 4)
> -      // Based on the docs of the webNavigation.onCommitted event, it should be raised when:
> +    // Based on the docs of the webNavigation.onCommitted event, it should be raised when:
> -      // "The document  might still be downloading, but at least part of
> +    // "The document  might still be downloading, but at least part of
> -      // the document has been received"
> +    // the document has been received"
> -      // and for some reason we don't fire onLocationChange for the
> +    // and for some reason we don't fire onLocationChange for the
> -      // initial navigation of a sub-frame.
> +    // initial navigation of a sub-frame.
> -      // For the above two reasons, we process the document change here and
> +    // For the above two reasons, when the navigation event is related to
> +    // a sub-frame we process the document change here and
> -      // then send an "Extension:DocumentChange" message to the main process,
> +    // then send an "Extension:DocumentChange" message to the main process,
> -      // where it will be turned into a webNavigation.onCommitted event.
> +    // where it will be turned into a webNavigation.onCommitted event.
> +    if ((webProgress.DOMWindow.top != webProgress.DOMWindow) &&
> +        (stateFlags & Ci.nsIWebProgressListener.STATE_IS_DOCUMENT)) {
>        this.sendDocumentChange({webProgress, locationURI});
>      }
>    },

For this reason I'm re-introducing the restriction of this workaround to the non top-level frames (which currently will not being able to detect the "server_redirect" transition).

::: toolkit/modules/addons/WebNavigationContent.js:95
(Diff revisions 3 - 4)
> +    } else if (webProgress.DOMWindow.top == webProgress.DOMWindow) {
> +      // Unfortunately to detect server redirection, we have to catch the
> +      // document changes from top level frames here.
> +      this.sendDocumentChange({webProgress, locationURI, request});

To be able to detect the "server_redirect" transition at least when it happens in top-level frames, I'm moving back the handling of the "Extension:DocumentChange" event into the "onLocationChange" method.
Assignee

Updated

3 years ago
Attachment #8738982 - Flags: feedback?(aswan)
Assignee

Updated

3 years ago
Blocks: 1264936
Attachment #8738982 - Flags: feedback?(aswan) → feedback+
Assignee

Comment 15

3 years ago
Comment on attachment 8738982 [details]
MozReview Request: Bug 1262794 - [webext] webNavigation refactoring and fix missing onCommitted event on iframes loading. r=krizsa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45003/diff/4-5/
Attachment #8738982 - Flags: feedback+
Assignee

Comment 16

3 years ago
(In reply to Luca Greco [:rpl] from comment #15)
> Comment on attachment 8738982 [details]
> MozReview Request: Bug 1262794 - [webext] webNavigation refactoring and fix
> missing onCommitted event on iframes loading. r=krizsa
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/45003/diff/4-5/

Minor tweak to the inline comments (add reference to the related bugzilla issue number).
Assignee

Updated

3 years ago
Keywords: checkin-needed
Assignee

Updated

3 years ago
Blocks: 1246125

Comment 18

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2f58e7ce1ab8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.